feat(tui): remove tool group borders and collapse completed tool results - #5003
Conversation
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR! The direction looks aligned with the TUI optimization epic (#4588 Track 3), and the changes themselves are focused and clean.
However, the PR body doesn't follow the pull request template. The template requires specific sections that help reviewers assess the change efficiently:
- What this PR does / Why it's needed — the current "Summary" and "Context" sections cover similar ground, but the template headings make it easier to scan.
- Reviewer Test Plan — needs "How to verify" steps, "Evidence (Before & After)" with actual screenshots or tmux output (not just ASCII mockups), and a "Tested on" OS table.
- Risk & Scope — important for understanding what's out of scope or could break.
- Linked Issues —
Closes #4588is mentioned in the body, but should be under this heading. - 中文说明 — the
<details>block with a Chinese translation is required.
Could you restructure the PR body to follow the template? The technical content is already good — it's mostly a reformatting exercise. Once that's done, happy to continue the review.
中文说明
感谢贡献!方向与 TUI 优化 epic(#4588 Track 3)对齐,改动本身也很聚焦。
不过 PR 正文没有按照 PR 模板 来写。模板要求以下章节,方便 reviewer 高效评估:
- What this PR does / Why it's needed — 当前的 "Summary" 和 "Context" 涵盖了类似内容,但模板标题更易于浏览。
- Reviewer Test Plan — 需要 "How to verify" 步骤、"Evidence (Before & After)" 附实际截图或 tmux 输出(不只是 ASCII 示意图),以及 "Tested on" 操作系统表格。
- Risk & Scope — 帮助了解哪些不在范围内或可能受影响。
- Linked Issues —
Closes #4588已在正文中提到,但应放在此标题下。 - 中文说明 — 需要包含中文翻译的
<details>块。
请按模板重新组织 PR 正文即可。技术内容本身已经不错,主要是格式调整。完成后可以继续 review。
— Qwen Code · qwen3.7-max
DragonnZhang
left a comment
There was a problem hiding this comment.
Test breakage: existing ToolMessage and ToolGroupMessage tests are not updated for the new collapse behavior.
The new shouldDefaultCollapse logic collapses completed tools (status === Success) by default, but several existing tests use baseProps with status: ToolCallStatus.Success and assert that results are visible. These will fail:
-
ToolMessage.test.tsx~line 155 —'renders basic tool information'assertstoContain('MockMarkdown:Test result'). WithshouldDefaultCollapse = true(sincestatus === SuccessandforceShowResultis not set), the result block will be hidden. -
ToolMessage.test.tsx~line 257 — Diff rendering test assertstoMatch(/MockDiff:--- a\/file\.txt/). Same issue —basePropshasSuccessstatus so result is collapsed. -
ToolMessage.test.tsx~line 265 — Saved-session preview test assertstoContain('MockDiff:--- file.txt'). Same root cause. -
ToolGroupMessage.test.tsxlines 486–523 — The entire'Border Color Logic'describe block tests border colors (yellow border when tools are pending,yellow border for shell commands,gray border when all successful) that no longer exist after border removal.
Suggested fix: either pass forceShowResult={true} in tests that intentionally need to verify result rendering, or override status to a non-Success value for those tests. The border-color test block should be removed or replaced with assertions on the new borderless layout.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
cf88bc2 to
43f8e30
Compare
wenshao
left a comment
There was a problem hiding this comment.
Additional suggestions (not on diff lines):
-
Stale comment in
ToolGroupMessage.tsx:459: the comment references the old compact-mode gate(!compactMode || forceShowResult ? renderer : 'none'), but the actual gate inToolMessage.tsxis nowshouldCollapse = compactMode && isCompleted && !forceShowResult. Update to match the new three-part formula. -
Stale test names:
describe('Border Color Logic', ...)and tests like'uses yellow border when tools are pending'inToolGroupMessage.test.tsxstill reference borders that were removed. Rename to reflect what is now verified (e.g.,'Status-dependent rendering').
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
There was a problem hiding this comment.
Clean TUI visual cleanup: removes borders from tool groups, dims completed tool names, and collapses tool results in compact mode. The width={contentWidth} constraint is preserved (protects against Ink rendering bugs). innerWidth and staticHeight adjustments correctly account for removed border (2 chars border + 2 chars padding → 2 chars padding only). Snapshot updates are consistent. LGTM ✅ — claude-opus-4-6 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
2 additional suggestions in unchanged code (not on diff lines):
[Suggestion] ToolGroupMessage.tsx:458 — Stale comment references the old compact-mode gate expression (!compactMode || forceShowResult ? renderer : 'none'). The actual logic in ToolMessage.tsx is now shouldCollapse = compactMode && isCompleted && !forceShowResult, which adds an isCompleted condition. Update the comment to reference the new expression so future maintainers aren't misled.
[Suggestion] ToolGroupMessage.test.tsx:506 — describe('Border Color Logic') block and its test names ("uses yellow border when tools are pending", "uses gray border when all tools are successful") still reference border colors that were removed in this PR. Consider renaming to describe what they actually verify now (e.g., 'Visual Layout' / "renders tool status indicators when tools are pending").
— qwen3.7-max via Qwen Code /review
|
@copilot resolve the merge conflicts in this pull request |
Conflicts are resolved in merge commit |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: Well-scoped TUI polish — removing borders and collapsing completed tool output in compact mode. Directly addresses the real pain point of TUI noise during multi-tool turns. Aligned with the project's TUI optimization track (#4588). Claude Code has similar collapsed-view improvements, confirming this is relevant UX work. Clear direction fit. On approach: The diff is now clean and focused — 7 files, all directly related to border removal and tool collapse. The formatting churn across unrelated files from the earlier revision has been resolved. The core changes are minimal: border removal from 3 container components, Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:范围合理的 TUI 打磨——移除边框和折叠已完成工具输出。直接解决多工具调用时 TUI 嘈杂的真实痛点。与项目 TUI 优化方向(#4588)一致。Claude Code 也有类似的折叠视图改进,确认这是相关的 UX 领域。方向明确对齐。 方案:diff 现在干净且聚焦——7 个文件,全部与边框移除和工具折叠直接相关。早期版本中无关文件的格式化噪音已解决。核心改动最小化:3 个容器组件移除边框、 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: Remove Comparison: The PR's approach matches my proposal exactly. All concerns from the prior review round are resolved:
Minor (non-blocking): No critical issues found. The diff is focused, the logic is correct, and the prior review feedback has been fully addressed. TUI Testing (tmux)Before (installed build v0.18.5 — rounded borders visible)After (this PR via
|
VerdictThis PR is ready to merge. All concerns from the prior review round have been addressed:
The feature itself is clean and well-scoped: border removal from 3 containers, The only remaining item is a stale doc comment at The 中文说明结论本 PR 可以合并。前一轮审查的所有问题已解决:
功能本身干净且范围合理:3 个容器移除边框、 唯一遗留项是 前一轮审查的 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Verdict
The feature itself is solid — borders are cleanly removed, the collapse logic is correct, and the before/after confirms it works as intended. This is the kind of focused UX improvement that makes multi-tool turns significantly less noisy.
The reason I'm not approving is the gap between what the PR says and what it does. The PR description explicitly states "no behavioral change" in non-compact mode, but the isDim dimming applies unconditionally to all completed tools — compact or not. That's not a bug, it's an undocumented behavioral change. Either it's intentional and the description needs updating, or it's unintentional and needs a compactMode gate.
The formatting churn across 8+ unrelated files and the stale test names are cleanup items — not hard blockers, but they make the diff harder to review than it needs to be.
Once the dimming behavior is clarified and the description is accurate, this looks ready to ship.
中文说明
结论
功能本身很好——边框干净移除,折叠逻辑正确,before/after 确认它按预期工作。这是那种让多工具调用轮次显著减少噪音的聚焦 UX 改进。
没有批准的原因是 PR 描述和实际行为之间的差距。PR 描述明确声称非紧凑模式"无行为变化",但 isDim 变暗无条件地应用于所有已完成工具——无论是否紧凑模式。这不是 bug,而是未记录的行为变化。要么是预期的需要更新描述,要么是非预期的需要添加 compactMode 门控。
8+ 个无关文件的格式化噪音和过时的测试名是清理项——不是硬性阻碍,但让 diff 比必要的更难审查。
一旦变暗行为明确且描述准确,看起来就可以合并了。
— Qwen Code · qwen3.7-max
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new review findings beyond what existing reviewers already identified — the functional changes (border removal, compact-mode collapse gate, innerWidth adjustment) are sound. Downgraded from Approve to Comment: CI still running. One minor suggestion: 11 of 16 changed files contain only Prettier formatting drift unrelated to the TUI changes; consider splitting those into a separate commit. — qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
Remove round borders from ToolGroupMessage, CompactToolGroupDisplay, and InlineParallelAgentsDisplay. Completed tools now default to a single collapsed header line with dimColor styling. Executing/error/confirming tools continue to show their full result block. Part of #4588 (Track 3: Simplify tool-call rendering). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Only collapse completed tool results in compact mode, preserving
full visibility in non-compact mode
- Subtract 2 from innerWidth to account for ToolMessage paddingX={1}
- Update snapshots to reflect removed borders
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Gate isDim on compact mode so non-compact tools stay fully styled
- Add paddingX={1} to CompactToolGroupDisplay for left-edge alignment
- Delete Border Color Logic test block (borders removed)
- Add compact-mode test coverage for Error/Executing/Pending/forceShowResult
- Clean up stale border references in comments
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
773900d to
6dcf188
Compare
| } | ||
| }, [emphasis]); | ||
| const isDim = compactMode && status === ToolCallStatus.Success; | ||
| return ( |
There was a problem hiding this comment.
[Suggestion] isDim is computed independently from shouldCollapse (line 675) and doesn't account for forceShowResult. When a completed tool is force-expanded in compact mode (e.g., terminal subagents via ToolGroupMessage line 462's isTerminalSubagentTool check), the result body is visible but the tool name stays dimmed and unbolded — a visual inconsistency between the header and body.
| return ( | |
| const isDim = compactMode && status === ToolCallStatus.Success && !forceShowResult; |
This requires threading forceShowResult into ToolInfo's props (add it to the interface at line 772). Alternatively, derive isDim from the parent's shouldCollapse and pass it down.
— qwen3.7-max via Qwen Code /review
| width={contentWidth} | ||
| borderColor={theme.border.default} | ||
| > | ||
| <Box flexDirection="column" width={contentWidth}> |
There was a problem hiding this comment.
[Suggestion] After removing the border, this memory-only branch and the main expanded view (line 393) have no paddingX on their outer <Box>. Meanwhile, CompactToolGroupDisplay correctly gained paddingX={1}. The old borderStyle="round" provided an implicit 1-char left offset that kept content aligned with the compact view. Without it, expanded-view content now sits 1 column to the left of compact-view content.
| <Box flexDirection="column" width={contentWidth}> | |
| <Box flexDirection="column" width={contentWidth} paddingX={1}> |
Apply the same to line 393: <Box flexDirection="column" width={contentWidth} paddingX={1} gap={0}>.
Note: this would also affect the innerWidth calculation — with outer paddingX={1} (2 chars) plus ToolMessage's own paddingX={1} (2 chars), innerWidth should become contentWidth - 4 again.
— qwen3.7-max via Qwen Code /review
| @@ -412,22 +391,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({ | |||
| } | |||
|
|
|||
| return ( | |||
There was a problem hiding this comment.
[Suggestion] The old code included a comment explaining that width={contentWidth} protects against an Ink rendering bug where rapidly re-rendering boxes can tear or span extra lines. The border was removed (correctly), but the width constraint is still needed — and the comment explaining why was deleted entirely. A future maintainer who sees width={contentWidth} on a borderless <Box> may remove it as seemingly redundant, re-introducing the rendering glitch.
Consider adding a brief comment:
{/* width={contentWidth} prevents Ink flex overflow during rapid re-renders */}
<Box flexDirection="column" width={contentWidth} gap={0}>— qwen3.7-max via Qwen Code /review
| ? displayRenderer | ||
| : { type: 'none' as const }; | ||
|
|
||
| const isCompleted = status === ToolCallStatus.Success; |
There was a problem hiding this comment.
[Suggestion] isCompleted is a misleading name — it implies all terminal states (Success, Error, Canceled), but only checks Success. A future maintainer reading shouldCollapse = compactMode && isCompleted && !forceShowResult may assume Error/Canceled are also "completed" and remove the now-redundant forceShowResult for Error status in ToolGroupMessage.tsx, inadvertently causing error results to collapse.
| const isCompleted = status === ToolCallStatus.Success; | |
| const isSuccess = status === ToolCallStatus.Success; | |
| const shouldCollapse = compactMode && isSuccess && !forceShowResult; |
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
| }, [emphasis]); | ||
| const isDim = compactMode && status === ToolCallStatus.Success; |
There was a problem hiding this comment.
[Suggestion] isDim only dims Success tools in compact mode, but Canceled is also a terminal, unactionable state. A canceled tool in compact mode currently renders at full brightness + bold + strikethrough, which is more visually noisy than necessary. Consider dimming canceled tools alongside completed ones:
| const isDim = compactMode && status === ToolCallStatus.Success; | |
| const isDim = compactMode && (status === ToolCallStatus.Success || status === ToolCallStatus.Canceled); |
— qwen3.7-max via Qwen Code /review
✅ Real-TUI verification report (maintainer build)I built the real Setup
1. Border removal — the headline change (deterministic A/B)Full (non-compact) mode, shell tool group: Compact mode, model-initiated tool group ( Borders gone in every path ( 2. The current
|
| Mode | Raw escape around tool name | Meaning |
|---|---|---|
| Non-compact | ESC[1m Shell Command ESC[0m |
bold, NOT dimmed |
| Compact | ESC[2m Shell Command … |
dimmed |
So non-compact mode has no dimming and full result visibility — the PR description is accurate for the current head. The blocker warrants a re-review / dismiss.
3. Behavior change is sound (mutation test proves tests aren't vacuous)
Reverting the 3 source components to base while keeping the PR's tests:
- The 3 new tests fail:
shows result for Error / Executing / Pending status in compact mode(base hides all results in compact; the PR correctly now shows in-progress/error and collapses onlySuccess). - All border golden-snapshot tests fail (they encode the borderless output).
forceShowResult overridesand the pre-existinghides result output in compact modestay green (not mutation-sensitive) — expected.
4. innerWidth fix (reviewer test item 5)
contentWidth - 4 → contentWidth - 2 correctly drops only the 2-char border (the 2-char ToolMessage padding remains). A ~95-char shell output rendered at full width with no premature wrap/truncation.
Checks
- Unit: 72/72 pass (
ToolMessage+ToolGroupMessage). Onezh-localei18n test timed out under concurrent build load; passes in isolation (247 ms) — environmental, unrelated. - CI: Lint, Tests (macOS/Ubuntu/Windows), CodeQL — all green.
BLOCKED= the review gate only, not a failing check.
⚠️ One open item (maintainer's own prior request, still unaddressed)
ToolGroupMessage.tsx:459 — the comment still references the old gate (!compactMode || forceShowResult ? renderer : 'none'); the real gate is now shouldCollapse = compactMode && isCompleted && !forceShowResult. Doc-only, 1-line fix. (The stale Border Color Logic / yellow border test names from the same review are fixed.)
Recommendation
Functionally ready to merge. Suggest: (a) update the stale comment at :459 (trivial, can be a follow-up), and (b) re-review / dismiss the stale stage-3 CHANGES_REQUESTED — the isDim concern it cites is resolved in 6dcf188 and confirmed above.
中文版
✅ 真实 TUI 验证报告(维护者构建)
我从本 PR head(6dcf188)在独立 git worktree 中构建了真实的 qwen 二进制,在 tmux 里做了与 base 的确定性 A/B 对比,外加变异测试和单测/CI 核查。结论:功能与描述完全一致,CI 全绿,距离合并只差一处陈旧注释 + 一条已失效的 CHANGES_REQUESTED。
环境
- worktree 切到 PR head
6dcf188;base = merge-base02847d7。 npm ci && npm run build→ 退出码 0(全包类型检查通过——移除theme/SHELL_*后无死导入)。- 独立
HOME(auth 从本地 settings 拷入),每次运行切换compactMode,YOLO 模式让 shell 工具自动完成。 - A/B 方法:先抓 FIXED,再
git checkout <base> -- messages/*+ 重建 cli 得到 BASE 二进制,相同按键,difftmux capture-pane。工具组分别用 shell 模式(!cmd,isUserInitiated,无需模型)和模型发起(run_shell_command)两条渲染路径产生。
1. 移除边框——核心改动(确定性 A/B)
完整(非紧凑)模式,shell 工具组:
改前 (base) 改后 (本 PR)
$ echo hello-from-tui $ echo hello-from-tui
╭───────────────────────────────────╮ ✓ Shell Command echo hello-from-tui
│ ✓ Shell Command echo hello-from-… │ hello-from-tui
│ hello-from-tui │
╰───────────────────────────────────╯
紧凑模式,模型发起的工具组(CompactToolGroupDisplay):
改前 (base) 改后 (本 PR)
╭──────────────────────────────────────────╮ ✓ Shell echo MODELTOOL-MARKER (…)
│✓ Shell echo MODELTOOL-MARKER (…) │ Press Ctrl+O to show full tool output
│Press Ctrl+O to show full tool output │
╰──────────────────────────────────────────╯
所有路径的边框都已移除(ToolGroupMessage 展开视图、CompactToolGroupDisplay,以及——通过源码/grep 核实、未做截图——InlineParallelAgentsDisplay 和 memory-only 组)。构建产物确认:FIXED 中 borderStyle 出现 0 次,BASE 中仍存在。防撕裂的 width={contentWidth} 约束保留;未观察到撕裂。
2. 当前的 CHANGES_REQUESTED 阻塞已失效——已验证修复
stage-3 review(2026-06-18)指出 isDim「对所有已完成工具生效——不分紧凑与否」,与「非紧凑模式无行为变化」相矛盾。这对它当时 review 的提交(b4e2d9b:isDim = status === Success)确实成立,但最终提交 6dcf188 已改为:
const isDim = compactMode && status === ToolCallStatus.Success;在真实 TUI 中用 escape 抓取已完成工具名加以证明:
| 模式 | 工具名附近的原始转义码 | 含义 |
|---|---|---|
| 非紧凑 | ESC[1m Shell Command ESC[0m |
加粗,未变暗 |
| 紧凑 | ESC[2m Shell Command … |
变暗 |
即:非紧凑模式既不变暗、结果也完整可见——PR 描述对当前 head 是准确的。该阻塞应重新 review / dismiss。
3. 行为变化是正确的(变异测试证明测试非空过)
把 3 个源组件回退到 base、保留 PR 的测试:
- 3 个新测试失败:
shows result for Error / Executing / Pending status in compact mode(base 在紧凑模式下隐藏所有结果;PR 正确地改为显示进行中/出错,仅折叠Success)。 - 全部边框 golden-snapshot 测试失败(它们记录的是无边框输出)。
forceShowResult overrides和原有的hides result output in compact mode仍绿(对变异不敏感)——符合预期。
4. innerWidth 修复(审查测试项 5)
contentWidth - 4 → contentWidth - 2 正确地只扣掉 2 字符边框(ToolMessage 的 2 字符 padding 保留)。一条约 95 字符的 shell 输出以完整宽度渲染,无提前换行/截断。
核查
- 单测:72/72 通过(
ToolMessage+ToolGroupMessage)。一个zh-localei18n 测试在并发构建压力下超时;单独跑通过(247ms)——环境因素,与本 PR 无关。 - CI:Lint、Tests(macOS/Ubuntu/Windows)、CodeQL——全绿。
BLOCKED仅是 review 闸门,不是测试失败。
⚠️ 一处遗留项(维护者此前的请求,仍未处理)
ToolGroupMessage.tsx:459 的注释仍引用旧门控 (!compactMode || forceShowResult ? renderer : 'none');实际门控已是 shouldCollapse = compactMode && isCompleted && !forceShowResult。纯文档,1 行即可修。(同一条 review 中的陈旧测试名 Border Color Logic / yellow border 已修复。)
建议
功能上可以合并。建议:(a) 更新 :459 的陈旧注释(很小,可作为 follow-up);(b) 重新 review / dismiss 已失效的 stage-3 CHANGES_REQUESTED——其引用的 isDim 问题已在 6dcf188 解决并经上文确认。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — all prior review concerns resolved. isDim properly gated on compactMode, diff is focused, tmux before/after confirms the feature works as described. Ready to ship. ✅
What this PR does
Removes the rounded border (
borderStyle="round") from tool group containers (ToolGroupMessage,CompactToolGroupDisplay,InlineParallelAgentsDisplay) and collapses completed tool result blocks in compact mode — showing only the single-line header (tool name + status indicator) instead of the full output.In non-compact (full) mode, completed tool results remain fully visible as before.
Why it's needed
The bordered groups and always-expanded tool results consume significant vertical space in the TUI, especially during multi-tool turns. Removing borders and collapsing completed results in compact mode gives a cleaner, denser output that lets users focus on in-progress and error tools. This is part of the TUI optimization effort tracked in #4588 (Track 3).
Reviewer Test Plan
How to verify
/compact): completed tools should show only the header line (status icon + tool name + elapsed time). Executing/error/confirming tools should still show full results.Evidence (Before & After)
N/A — TUI rendering change, reviewer can verify locally with any multi-tool prompt.
Tested on
Risk & Scope
Linked Issues
Closes #4588
中文说明
本 PR 做了什么
移除了工具组容器(
ToolGroupMessage、CompactToolGroupDisplay、InlineParallelAgentsDisplay)的圆角边框(borderStyle="round"),并在紧凑模式下折叠已完成工具的结果——仅显示单行标题(工具名 + 状态图标),不再展示完整输出。在非紧凑(完整)模式下,已完成工具的结果仍然完整可见,行为不变。
为什么需要
带边框的工具组和始终展开的工具结果在 TUI 中占据大量垂直空间,尤其在多工具调用时。移除边框并在紧凑模式下折叠已完成结果,可以让输出更简洁、密集,用户能专注于进行中和出错的工具。这是 #4588(Track 3)TUI 优化工作的一部分。
审查测试计划
如何验证
/compact):已完成工具应仅显示标题行(状态图标 + 工具名 + 耗时),执行中/出错/确认中的工具仍显示完整结果。测试环境
风险与范围
关联 Issue
Closes #4588